Repository navigation
Expose the agent provider contract for all agent types - #22
oznetmaster merged 3 commits into
Conversation
Co-authored-by: OpenAI Codex <noreply@openai.com>
CharliePoole
left a comment
There was a problem hiding this comment.
Let's spell everything out in the interface. I'll send you some details about agent implementation that I referred to separately.
| /// test agents, which satisfy the criteria specified in a TestPackage. | ||
| /// The engine loads implementations through /NUnit/Engine/AgentProviders. | ||
| /// Capability queries do not acquire execution ownership. An acquired agent | ||
| /// remains associated with its provider until ReleaseAgent succeeds. |
There was a problem hiding this comment.
I would remove the sentence "The engine loads..." It sounds as if this the path is an extension point path, while it's just the relative location of the source code and not needed here. The "Capability queries statement could be moved to IsAgentAvailable or left here as you prefer. However, it needs to be qualified, perhaps in a "Remarks" section, perhaps in a separate document to which we refer, perhaps both.
Agents can be written by anyone. There is a mechanism to enquire regarding certain capabilities without instantiating the engine, provided the author uses that mechanism. In the early V4 code, that mechanism didn't exist. It was added later and the four existing V4 agents make use of it. However, the engine doesn't require that it be used and falls back to loading the extension into memory and instantiating the engine if the necessary attributes are not provided.
I realize that the above is not an adequate explanation, but let's decide where such an explanation should live and I'll write one.
| /// <param name="package">The test package to be run</param> | ||
| /// <returns>An ITestAgent</returns> | ||
| /// <exception cref="ArgumentException">If the package is invalid.</exception> | ||
| /// <exception cref="InvalidOperationException">If no compatible agent is available.</exception> |
There was a problem hiding this comment.
Add something under InvalidOperationException to say you should check first using IsAgentAvailable I also think this needs a remark about ownership
Qualify discovery and capability queries, document acquisition and failed release responsibility, and apply resource cleanup guidance to all providers. Remove the obsolete provider-loading path claim without changing signatures. Prepared by Neil Colvin with assistance from OpenAI Codex.
CharliePoole
left a comment
There was a problem hiding this comment.
I like the detailed explanation of who is responsible for what. We may want to have more details somewhere, particularly regarding how agents work with respect to queries, i.e. sometimes needing instantiation, sometimes not. I think it's ready to merge but I'd like a second review since it's pretty important.
|
@manfred-brands Even though the code is small, this interface and it's comments describe how we want the engine to acquire and interact with agents in the future. It was previously an internal interface used only for the single type of agent we supported up to now. We think this will work for all three types: in-process, local process and remote. It's due to the remote agents, supplied by a server, that we need to make this a public external interface. Because we now use multiple agent providers we have introduced the notion that each provider has ownership of the agents it supplies and needs to take care of any cleanup when they are released. Bottom line, this is a design review as well as a code review. Neil and I did the design, so I think your separate review is a good idea in case we missed anything. |
manfred-brands
left a comment
There was a problem hiding this comment.
The idea and the described operation are fine.
I do have some remarks about (inconsistent) terminology.
| /// imply that the caller owns its process, transport or deployment. | ||
| /// </para> | ||
| /// <para> | ||
| /// If acquisition throws, the caller has no returned agent to release. The provider |
There was a problem hiding this comment.
That is obvious as there is no return value to pass into ReleaseAgent.
I suggest to remove the first part.
| /// If acquisition throws, the caller has no returned agent to release. The provider | |
| /// If acquisition throws, the provider |
| /// <para> | ||
| /// If acquisition throws, the caller has no returned agent to release. The provider | ||
| /// remains responsible for resources created during the failed attempt and for | ||
| /// reconciling an outcome that cannot yet be confirmed. |
There was a problem hiding this comment.
reconciling an outcome that cannot yet be confirmed.
Note sure what that means?
There was a problem hiding this comment.
@oznetmaster wrote
By an unconfirmed acquisition outcome I meant, for example, that the remote host creates an agent but its reply is lost. GetAgent can then fail even though an agent exists on the host. The provider must determine what was created and handle its ownership and cleanup; the caller cannot assume that the failed call created nothing. I will replace “reconciling an outcome” with that concrete explanation, including equivalent wording for a failed return.
I think any such concrete explanation should indicate that it applies to remote providers.
There was a problem hiding this comment.
Yes, I think that whole sentence can be left out. The first part is enough to say that the provider is responsible for cleaning up.
| /// Returns a previously acquired test agent to the provider that supplied it. | ||
| /// </summary> | ||
| /// <param name="agent">An agent previously provided by a call to GetAgent.</param> |
There was a problem hiding this comment.
We should be consistent in terminology and not change from acquired to provided.
| /// Returns a previously acquired test agent to the provider that supplied it. | |
| /// </summary> | |
| /// <param name="agent">An agent previously provided by a call to GetAgent.</param> | |
| /// Returns a previously acquired test agent to the provider that supplied it. | |
| /// </summary> | |
| /// <param name="agent">An agent previously acquired by a call to GetAgent.</param> |
| /// reconciling an outcome that cannot yet be confirmed. | ||
| /// </para> | ||
| /// </remarks> | ||
| ITestAgent GetAgent(TestPackage package); |
There was a problem hiding this comment.
I have no problem with Get but the help text on the Release method calls about acquired agent.
So maybe rename this to AcquireAgent?
| /// that the agent no longer needs them. This applies to all provider types. | ||
| /// </para> | ||
| /// </remarks> | ||
| void ReleaseAgent(ITestAgent agent); |
There was a problem hiding this comment.
Terminology.
Release to me implies Disposal which is explicitly denied in the last paragraph.
But I might be biased by the IFactory in Castle.Windows we used before MS DI.
Would ReturnAgent be a better term?
The interface comments talks about returning an agent.
The summary says returns an agent
Later on it becomes releasing, but it is stated that the provider can return the same agent for another call and calling ReleaseAgent does not mean releasing resources.
This interface is like a pool of agents, MS uses Get/Return for ObjectPool and uses Rent/Return for ArrayPool
There was a problem hiding this comment.
I agree, let's go with ReturnAgent.
There was a problem hiding this comment.
Maybe drop the statement that the provider can return the same agent. Is there any reason the caller needs to know that?
|
@manfred-brands @CharliePoole, thank you. I agree with simplifying the redundant sentence and consistently using “acquired” in the parameter documentation. By an unconfirmed acquisition outcome I meant, for example, that the remote host creates an agent but its reply is lost. GetAgent can then fail even though an agent exists on the host. The provider must determine what was created and handle its ownership and cleanup; the caller cannot assume that the failed call created nothing. I will replace “reconciling an outcome” with that concrete explanation, including equivalent wording for a failed return. On naming, I lean toward GetAgent/ReturnAgent. GetAgent can acquire an existing or newly created agent; ReturnAgent clearly ends the caller's right to use it while leaving the provider to decide whether to reuse it or stop it. This follows the Get/Return terminology you cited without requiring every provider to implement a pool. AcquireAgent/ReturnAgent would also work if the team prefers the more explicit acquisition name. I suggest agreeing the method names here before changing the public contract and engine call sites together. I will keep the existing signatures until that is settled. Prepared by Neil Colvin with assistance from OpenAI Codex. |
|
Let's go with In the comment text, I'd like to retain the existing emphasis on the client not knowing whether a provider is a factory, a pool of agents or something else and not knowing what resources it manages. On the client side, we can talk about "getting," "acquiring" and "obtaining" without confusion but should use "return" consistently. We should always refer to the provider as a "provider" unless somebody can think of a word that's synonymous rather than having a more specific meaning as do "factory" and "pool." The provider "provides" agents or "returns" them. It may or may not create them, for example. For reference purposes... we currently recognize three types of agents -- there could be more some day -- as follows...
General notes on reuse...
@oznetmaster Is this enough for you to update? We'll then re-review and hopefully merge. |
|
Although not part of this PR, the talk of naming makes me think of this question... I removed the (unused) Maybe I am hanging on to the "agency" metaphor unnecessarily. Should we rename |
Adopt the reviewed ReturnAgent name and keep provider implementation and reuse policy opaque to callers. Simplify failed-acquisition responsibility and consistently describe returning acquired references. Prepared by Neil Colvin with assistance from OpenAI Codex.
|
@CharliePoole @manfred-brands, yes, that is enough to update. I have prepared GetAgent/ReturnAgent with "return" used consistently. I removed the redundant acquisition sentence and the unclear outcome wording, leaving the provider's cleanup responsibility stated directly. I also removed the reuse/pool details so callers need not know how the provider manages agents or resources. The revised API builds cleanly, its 69 existing tests pass on each runtime, and an independent provider/caller compiles against the new names on both targets. The engine follow-up will need to remove its internal duplicate and adopt ReturnAgent alongside the agreed published API package. On the separate coordinator question, I favor TestAgentService: it describes its present role as a service composing providers. I would make that a separate engine refactor, retaining the current provider composition and launcher/registration contracts. Prepared by Neil Colvin with assistance from OpenAI Codex. |
CharliePoole
left a comment
There was a problem hiding this comment.
This looks ready to merge as far as I can see. Of course, it may change a few times before release due to our lack of omniscience. :-) Please wait for an OK from @manfred-brands as well before merging.
manfred-brands
left a comment
There was a problem hiding this comment.
Thanks @oznetmaster and @CharliePoole.
Looks good, descriptive and clear.
Move ITestAgentProvider into NUnit.Engine.Api so in-process, local-process and remote providers can share one public contract. Keep its NUnit.Engine.Services namespace alongside ITestAgentInfo, with IsAgentAvailable, GetAgent and ReturnAgent. No provider-discovery mechanism or remote-execution implementation is introduced here.
Following Charlie's and Manfred's reviews, use GetAgent / ReturnAgent and document the boundaries that callers need:
The latest follow-up intentionally renames ReleaseAgent to ReturnAgent on the new interface under review and revises its XML comments. This is not a documentation-only change. The other declarations, target frameworks and package dependencies are unchanged. It changes no existing released member; ITestAgentProvider is being added by this PR.
Integration and scope
After publication of an agreed API package containing this interface, the engine follow-up must remove its internal duplicate, consume that actual package and update provider/caller implementations to ReturnAgent together. This gives the engine and independently compiled providers one contract identity. It should retain the current coordinator/provider composition rather than revive the superseded #1917 implementation. No unpublished package version or private feed is committed here.
Provider construction/loading, remote transport and deployment, and the separate proposal to rename the engine coordinator TestAgency to TestAgentService are outside this API PR. Existing launcher and registration contracts are unchanged by this patch. The engine integration will need its own build and behavior validation on the coordinated public-package base.
Validation on Windows
No fresh engine integration, Linux/macOS, GUI, adapter or hardware result is claimed for this API follow-up. Earlier prototype results belong to their original dependency graph.
Prepared by Neil Colvin with assistance from OpenAI Codex.